(grid): rename pruneStaleSubagents so sidebar.js stops shadowing it - #137
Merged
Conversation
JeanBaptisteRenard
added a commit
that referenced
this pull request
Aug 21, 2026
…debar.js grid-view.js and sidebar.js both declared a top-level function pruneStaleSubagents() as classic <script> tags sharing one global scope; sidebar.js loads after grid-view.js (index.html:129 vs :133), so its declaration silently won the shared global name. wrapInGridCard()'s call site therefore ran sidebar's prune (on activeSubagentsByParent) instead of grid-view's own (on activeSubagents) — the grid's 60s TTL never fired, so a pill could stay "running" forever if the parent PTY died before the matching subagent-completed event. Renamed grid-view's function to pruneStaleGridSubagents and fixed the stale "called on each grid render cycle" comment while touching this block (PR #137 review, Finding 1 + Finding 2). Adds a regression test that loads grid-view.js then sidebar.js in the real index.html script order and drives wrapInGridCard()'s TTL prune end to end — pinned red against the shared name, green after the rename.
JeanBaptisteRenard
enabled auto-merge (squash)
August 21, 2026 15:00
Renderer files load as plain non-module scripts sharing one global scope, and index.html loads sidebar.js (:136) after grid-view.js (:132). Both declared a top-level pruneStaleSubagents(), so the later declaration won: wrapInGridCard() was calling sidebar.js's prune against activeSubagentsByParent instead of the grid's own activeSubagents map. Two consequences: a grid subagent pill whose parent PTY died before the subagent-completed event stayed "running" forever, and every grid card re-wrap triggered a spurious prune of the sidebar's map. Rename the grid-side function; sidebar.js is untouched. The new test pins both the absence of colliding top-level names between the two files and the end-to-end TTL prune through the real wrapInGridCard() call site.
devsuitup
force-pushed
the
fix/grid-subagent-pills
branch
from
August 23, 2026 20:29
6675576 to
8bd37bc
Compare
devsuitup
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Renderer files load as plain non-module scripts sharing one global scope, and
index.htmlloadssidebar.js(:136) aftergrid-view.js(:132). Both declared a top-levelpruneStaleSubagents(), so the later declaration won: grid-view'swrapInGridCard()was pruning the sidebar'sactiveSubagentsByParentmap instead of the grid's ownactiveSubagents.A grid subagent pill therefore stayed "running" forever whenever the parent PTY died before the matching
subagent-completedevent, and every grid card re-wrap fired a spurious prune of the sidebar's map.Renames the grid-side function only;
sidebar.jsis untouched.The listener-arity half of this PR's original scope shipped separately in #142, so this branch was rebuilt on current main and now carries the rename alone.
test/dom-grid-sidebar-prune-collision.test.jspins both the absence of colliding top-level names across the two files and the end-to-end TTL prune through the realwrapInGridCard()call site. Both assertions fail on main without the rename: